-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Strip mentions from forwarded messages #30884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Strip mentions from forwarded messages #30884
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suboptimal — should probably actually recalculate the m.mentions
based on message body (without reply relation), but not entirely clear to me what 'correct' design would be
As there is no EditorModel, attachMentions() currently does nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs forwarded messages through attachMentions()
— which just effectively sets m.mentions
to empty, as there is no EditorModel
for the forwarded message. This feels like the least-bad way to fix #30883 in alignment with the existing codebase, while leaving room for future improvements. Actually determining the m.mentions
based on the message body would likely require some degree of structural changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you're working on tests - thanks. This could probably do with a test itself.
}; | ||
|
||
const transformEvent = (event: MatrixEvent): { type: string; content: IContent } => { | ||
const transformEvent = (event: MatrixEvent, userId: string): { type: string; content: IContent } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was just about squeaking by without doc as an internal function, although it still wasn't super clear in what way it's transforming it. Now it has another param, I think it really does need doc as I have absolutely no guesses as to what this function might do with the user ID it's been given.
}; | ||
} | ||
|
||
// Force an empty m.mentions property as there is no EditorModel to parse pills from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are explicitly forwarding an event here, I'd make this comment closer to what you have in the PR description, ie. talking about the fact we're stripping mentions from a forwarded message and why.
Fixes #30883
Checklist
public
/exported
symbols have accurate TSDoc documentation.